Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API-factory: Method to Associate needed entities, then Register the Host #14974

Closed
wants to merge 1 commit into from

Conversation

damoore044
Copy link
Contributor

@damoore044 damoore044 commented May 6, 2024

Problem Statement

Many hosts for Errata-UI, use a fixture registered_contenthost (@L223), which registers a rhel_contenthost fixture.
In parameterized sessions, the registered_contenthost can fail if preexisting content is present, so decent cleanup was needed.

  • Current Errata-UI reg. hosts are setup as a fixture of a fixture,
  • intermittent setup failures in the fixture, or mismatched/persisted content in CI runs.
  • We do not seem to have a 6.15+ setup method like the older repos_collection.setup_virtual_machine() or ContentHost.contenthost_setup().

Solution

Create an API Factory method (could also exist in the ContentHost class?), that will accept desired entities to host content, associate them, then register the desired client (ie a passed fixture rhel_contenthost) to the now attached entities, using ak.

Note: For api-factory method, you can pass id (int), name (string), or instance for entities (except for client, must be the entity's instance, and except for boolean args)

Very minimal setup is needed, create the needed custom entities or import fixtures, add repos to CV prior if desired, then call the method. Prior to calling, you can also do varying amounts of custom setup to lce, CV etc. For CV, latest version (if any) will be used, can handle entities partially associated or fully.

  • To enable repos for host, we can add them to the contentview like I did to setup test_end_to_end, then call the new function. Once registered, I enable them within the method by flag, and the repos (custom or redhat) will be visible to the host, from CV associated to AK. Can also skip enable_repos for method (default), and do it yourself after.
  • Alternatively, using the method you can register to an empty CV, then add & sync repos in the CV, publish/promote, enable repos, content is now available to the registered client.
  • You can also call other setup methods, like setup_org_for_a_custom_repo or setup_rh_repo_and_return_id etc,
    then simply add the repos to CV, before calling this new method. The content from repo(s) is associated and visible as expected.
  • The method makes use of unittest MagicMock(), to mock a request to add finalizer (cleanup method to unregister host during teardown).
  • You can register the host to entities using names: org, ak, env, cv
    ie: can accept str "Library" for environment, and can accept "Default Organization View" for content-view.
    Do not pass the Library env as an id or instance, this will fail (building a check for this). Done
    (ref ~@ln845)

Related Issues

  • CI Intermittent UI failures of registered_contenthost, fails setup, or mismatched content, between parameters of a combined session.
  • Difficulty creating/fixing tests, as many useful fixtures need varying levels of setup before they can be used to host content.

Demonstration-PRT Case

  • UI::Errata::test_end_to_end : Implemented to replace existing registered_contenthost local fixture, which relies on convoluted setup to achieve a useable host ready for content/with content.
trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@damoore044 damoore044 added CherryPick PR needs CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 labels May 6, 2024
@damoore044 damoore044 self-assigned this May 6, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@damoore044 damoore044 force-pushed the errata-api-factory branch from cc8748f to fbfe2aa Compare May 6, 2024 13:15
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6801
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ============ 2 failed, 1 passed, 313 warnings in 2223.06s (0:37:03) ============

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label May 6, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@damoore044 damoore044 force-pushed the errata-api-factory branch 3 times, most recently from ac316f8 to 398ae70 Compare May 6, 2024 16:44
@damoore044 damoore044 removed the 6.14.z Introduced in or relating directly to Satellite 6.14 label May 6, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6809
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ================= 3 passed, 341 warnings in 2343.18s (0:39:03) =================

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels May 6, 2024
@damoore044 damoore044 force-pushed the errata-api-factory branch 2 times, most recently from e2e4386 to 9f44f90 Compare May 7, 2024 13:25
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 7, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6828
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ================= 3 failed, 233 warnings in 1776.62s (0:29:36) =================

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label May 7, 2024
@damoore044 damoore044 force-pushed the errata-api-factory branch 3 times, most recently from 20ce1a4 to 8ac5e8f Compare May 7, 2024 14:31
@damoore044 damoore044 force-pushed the errata-api-factory branch from 9392427 to 50b1c42 Compare May 9, 2024 19:46
@damoore044 damoore044 requested a review from JacobCallahan May 9, 2024 19:46
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 9, 2024
@damoore044 damoore044 force-pushed the errata-api-factory branch from 50b1c42 to a1d45f7 Compare May 9, 2024 19:49
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6872
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ================= 3 passed, 325 warnings in 2280.56s (0:38:00) =================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 9, 2024
@damoore044 damoore044 force-pushed the errata-api-factory branch from a1d45f7 to fd1bdc6 Compare May 13, 2024 14:59
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 13, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6905
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ================= 3 passed, 328 warnings in 2452.15s (0:40:52) =================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 13, 2024
Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work in good direction I think! I've just put a few suggestions/questions to consider.

* Add desired repos to the content-view prior to calling this helper.
Or, add them to content-view after calling, then publish/promote.

param satellite: sat instance where needed entities exist.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
param satellite: sat instance where needed entities exist.

Comment on lines 809 to 821
if not force and client.subscribed:
if unregister:
client.unregister()
else:
method_error['message'] = (
'Passed client is already registered.'
' Unregister the host prior to calling this method.'
' Or, pass unregister=True, to unregister within this method first,'
' Or, pass force=True, to bypass during registration.'
)
return method_error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the force=True option in registration basically performs the same under the hood - unregisters the host first and then registers it again. Do we really need this client.unregister() prior to L912 (forceful) registration for some reason I don't see right now? (Like for some setup between this line and L912?)
If not, we could be fine with the force option only.

Suggested change
if not force and client.subscribed:
if unregister:
client.unregister()
else:
method_error['message'] = (
'Passed client is already registered.'
' Unregister the host prior to calling this method.'
' Or, pass unregister=True, to unregister within this method first,'
' Or, pass force=True, to bypass during registration.'
)
return method_error
if not force and client.subscribed:
method_error['message'] = (
'Passed client is already registered.'
' Unregister the host prior to calling this method.'
' Or, pass force=True, to bypass during registration.'
)
return method_error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed in favor of unregistering the host, if subscribed, in any actual test or fixture that calls this method.
In parameterized sessions, the rhel ContentHost(s) can get reused and then will not re-register unless force=True.


if ( # publish a content-view-version if none exist, or needs_publish is True
len(entities['ContentView'].version) == 0
or entities['ContentView'].needs_publish is True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting use of the feature.

):
entities['ContentView'].publish()
# read updated entitites after modifying CV
entities = {k: v.read() for k, v in entities.items()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we need to read/update all the entities? Wouldn't be update of entities['ContentView'] enough?
Nice syntax anyway.

activation_keys=entities['ActivationKey'].name,
target=self._satellite,
org=entities['Organization'],
loc=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could pass the loc as parameter too (for future uses).

# ie: any repos added to content-view prior to calling this method.
# note: this will fail if no repos are available to client from CV/AK
if enable_repos:
output = client.execute(r'subscription-manager repos --enable \*')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we use this at several places already. I just wonder if it wouldn't be better to do content-override at the AK entity (based on the enable_repos param) prior to the client registration, so the repos would get enabled through the AK already.

@damoore044 damoore044 force-pushed the errata-api-factory branch from fd1bdc6 to cd1fb0d Compare May 14, 2024 12:45
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 14, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6920
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ================= 3 passed, 325 warnings in 2262.87s (0:37:42) =================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 14, 2024
@damoore044 damoore044 force-pushed the errata-api-factory branch from cd1fb0d to 2aa710c Compare May 17, 2024 14:17
@damoore044 damoore044 requested a review from a team as a code owner May 17, 2024 14:17
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label May 17, 2024
@damoore044 damoore044 force-pushed the errata-api-factory branch from 2aa710c to fa4274e Compare May 17, 2024 14:19
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py::test_end_to_end

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6993
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py::test_end_to_end --external-logging
Test Result : ================= 3 passed, 323 warnings in 2384.87s (0:39:44) =================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 17, 2024
@damoore044
Copy link
Contributor Author

damoore044 commented Jun 4, 2024

Work has been brought into #15085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.15.z Introduced in or relating directly to Satellite 6.15 CherryPick PR needs CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants